Use a C-safe return type for `__rust_[ui]128_*` overflowing intrinsics #134338

tgross35 · 2024-12-15T11:32:44Z

Combined with [1], this will change the overflowing multiplication operations to return an extern "C"-safe type.

Link: rust-lang/compiler-builtins#735 [1]

rustbot · 2024-12-15T11:32:53Z

rustbot has assigned @wesleywiser.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

rustbot · 2024-12-15T11:32:55Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo, @GuillaumeGomez

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

tgross35 · 2024-12-15T11:36:34Z

r? @bjorn3 @antoyo

There are a couple other places I need to update for GCC still.

tgross35 · 2024-12-15T11:50:09Z

For cranelift, is there a way to load the stack slot so it can be combined with the integer for a CValue::by_val_pair @bjorn3?

bjorn3 · 2024-12-15T14:28:45Z

You can use oflow_out_place.to_cvalue(fx).load_scalar(fx).

compiler/rustc_codegen_cranelift/src/compiler_builtins.rs

rustbot · 2024-12-17T22:07:07Z

These commits modify the library/Cargo.lock file. Unintentional changes to library/Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

bors · 2024-12-28T11:57:40Z

☔ The latest upstream changes (presumably #134844) made this pull request unmergeable. Please resolve the merge conflicts.

bjorn3 · 2025-01-09T12:34:08Z

compiler/rustc_codegen_cranelift/Cargo.toml

@@ -23,7 +23,8 @@ libloading = { version = "0.8.0", optional = true }
 smallvec = "1.8.1"

 [patch.crates-io]
-# Uncomment to use local checkout of cranelift
+# todo: remove patch
+compiler_builtins = { git = "https://github.com/tgross35/compiler-builtins.git", package = "compiler_builtins", branch = "overflowing-c-safe-ret" }


This doesn't apply to building the sysroot. Also the patch in library/Cargo.toml should be enough to apply while building a sysroot for cg_clif.

bjorn3 · 2025-01-09T12:36:15Z

compiler/rustc_codegen_cranelift/Cargo.lock

 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "b3d1d046238990b9cf5bcde22a3fb3584ee5cf65fb2765f454ed428c7a0063da"
+checksum = "c1fd03a028ef38ba2276dce7e33fcd6369c158a1bca17946c4b1b701891c1ff7"


The lockfile changes should be reverted.

Yeah, the entire second commit is just for testing and will be dropped. My plan is once you approve the Cranelift changes and somebody from GCC has looked that portion over, I will merge rust-lang/compiler-builtins#735, drop the second commit (which add my crates-iopatch), replace it with a normalcompiler-builtins` bump, and then this PR's merge will change everything atomically.

However, I am stuck completing the current bump at #135180, so I will hold off until that gets fixed.

tgross35 · 2025-01-09T21:03:01Z

@rust-lang/wg-gcc-backend would one of you mind taking a look at the GCC changes? I am assuming that since this bump means the intrinsics are changing to get the same signature (type aside) as __mulosi4 they no longer need to be special cased. However, I am not very familiar with this code or whether it gets tested in CI somehow.

antoyo · 2025-01-10T01:13:06Z

compiler/rustc_codegen_gcc/src/int.rs

-                            _ => unreachable!(),
-                        },
-                    };
-                    return self.operation_with_overflow(func_name, lhs, rhs);


From what I can see, this change doesn't look right as this will fall to calling self.overflow_call() below which expects that the overflow flag is returned from the function call (like these GCC intrinsics) whereas your change seems to return the integer result and not the overflow flag.

My guess is that you'll need to keep this special handling and change the method operation_with_overflow accordingly as these intrinsics are also used in src/intrinsic/mod.rs.

AFAIK, the only related tests that run in this CI are those lines.

Thanks for taking a look. Doesn't this block currently sometimes use __mulosi4 without an early turn? After the change to compiler_builtins, __rust_*128_o should be consistent with the __mulo*i4 functions.

AFAIK, the only related tests that run in this CI are those lines.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

Thanks for taking a look. Doesn't this block currently sometimes use __mulosi4 without an early turn? After the change to compiler_builtins, __rust_*128_o should be consistent with the __mulo*i4 functions.

Indeed, it looks like there might be a bug in here.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

What do you mean? There's more tests that are ran in the repo of rustc_codegen_gcc itself.

Indeed, it looks like there might be a bug in here.

I changed this so mulo and the __rust functions should now be taking the same codepath, still have to update the rest.

Is there any kind of codegen/asm test that should be added here? Or is the existing test sufficient once CI gets that far.

What do you mean? There's more tests that are ran in the repo of rustc_codegen_gcc itself.

I am just wondering whether I should add another test anywhere to verify the new behavior, especially if __mulosi4 may have been called incorrectly before. And if so, what format/location this should be (for LLVM I'd probably add a codegen test, but I'm not sure what the equivalent would be here).

I was expecting that there would be UI tests for these, so that every backend could benefit from them.
Do they have to be codegen tests?

There are integer tests here for the GCC codegen, but they are not ran in the Rust CI, as far as I know, so if it's too much of a pain, we can open an issue to remind me to add tests for this in the future.

Looking at https://rust.godbolt.org/z/eWfPK6rGW it seems like GCC lowers these operations to assembly (LLVM as well), which possibly explains why the test was not failing before even if the __mulosi4 call had a bug. I am not very familiar with this backend but I take it GCC intercepts the call to __builtin_sub_overflow and adjusts for the correct size?

If it doesn't emit this call then I am not sure what kind of test would work here, but can add something if you have any ideas. I updated the code and it should be ready for review, it builds but have not been able to run tests.

I was expecting that there would be UI tests for these, so that every backend could benefit from them.

Agreed that we should have something end-to-end here, possibly reusing the test generators from compiler-builtins/libm. I think the status quo assumption is that we don't need to test the lowering too heavily since that is the backend's responsibility, but it would still be good to have something systematic.

I am not very familiar with this backend but I take it GCC intercepts the call to __builtin_sub_overflow and adjusts for the correct size?

Yes. The doc mentions:

The first built-in function allows arbitrary integral types for operands and the result type must be pointer to some integral type other than enumerated or boolean type, the rest of the built-in functions have explicit integer types.

I reviewed your code: it looks good, thanks!

This test does call overflowing_mul, so I'm not sure why it wasn't tested there.
Any ideas?

This test does call overflowing_mul, so I'm not sure why it wasn't tested there. Any ideas?

In order to hit the branch where __mulosi/__mulodi gets emitted, the self.is_native_int_type(lhs.get_type()) condition has to fail for i32 or i64 - I'm guessing there probably aren't many platforms where that would happen. The calls to __rust_i128_* were correct before but it looks like is_native_int_type has to be returning true for i128 too, at least on x86, since that gets asm lowering https://rust.godbolt.org/z/jPfMK5vhv. I take it maybe the gcc-13-without-int128 CI jobs in the cg_gcc repo would cover this?

Yes, that CI job uses a GCC where 128-bit integers are disabled to test this indeed.

Right, that must be why it wasn't tested. But I think i64 is supported on all platforms, so I'll open an issue to check that.

antoyo · 2025-01-12T16:46:15Z

compiler/rustc_codegen_gcc/src/int.rs

+        let res_ty = match width {
+            32 => self.tcx.types.i32,
+            64 => self.tcx.types.i64,
+            128 => self.tcx.types.i128,
+            _ => unreachable!("unexpected integer size"),
+        };


Was there a reason to do that instead of reusing a_type?

I believe a_type is the GCC type and I need the rustc type to calculate the layout. I could not find a way to convert in this direction - is there one? Or a way to get a layout directly from the GCC type (feel like I might be missing something here).

Oh, sorry I forgot that you now have multiple types to handle here and not only 128-bit integers.
It's good as it is.

antoyo · 2025-01-12T16:46:42Z

compiler/rustc_codegen_gcc/src/int.rs

+        let oflow_param_type = oflow_type.make_pointer();
+        let res_type = a_type;
+
+        let oflow_value = self.current_func().new_local(self.location, oflow_type, "overflow");


Please rename all oflow to overflow to make it a bit clearer.

This is done

antoyo

This looks good to me.
Thanks a lot!

antoyo · 2025-01-12T20:30:05Z

compiler/rustc_codegen_gcc/build_system/build_sysroot/Cargo.toml

+# todo: remove patch
+compiler_builtins = { git = "https://github.com/tgross35/compiler-builtins.git", package = "compiler_builtins", branch = "NOMERGE-overflowing-c-safe-ret-version" }


Only this to remove, I guess.

tgross35 · 2025-01-12T20:35:03Z

Thanks for reviewing!

Actually merging this is still blocked until I can update compiler-builtins. Once that works again, I'll drop the Cargo patches, bump the version, and merge this.

@rustbot blocked

bors · 2025-01-15T03:17:26Z

☔ The latest upstream changes (presumably #135180) made this pull request unmergeable. Please resolve the merge conflicts.

Combined with [1], this will change the overflowing multiplication operations to return an `extern "C"`-safe type. Link: rust-lang/compiler-builtins#735 [1]

0.1.142 fixes an issue parsing optimization flags, and 0.1.143 changes `__rust_[ui]128_*` builtins to use a C-safe signature.

tgross35 · 2025-01-15T04:35:31Z

My diagnostics fix merged, which meant the r-l/rust update to builtins 0.1.141 could merge, which meant I could merge the signature changes to builtins, and now this is finally unblocked. Dropped the branch override patch and replaced it with a bump to builtins 0.1.143. Cranelift tests pass locally.

@bors r=bjorn3,antoyo

bors · 2025-01-15T04:35:34Z

📌 Commit f6a2db8 has been approved by bjorn3,antoyo

It is now in the queue for this repository.

rustbot assigned wesleywiser Dec 15, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 15, 2024

tgross35 changed the title ~~Use a C-safe return type for __rust_[ui]128_* overflowing intrinsics~~ [wip] Use a C-safe return type for __rust_[ui]128_* overflowing intrinsics Dec 15, 2024

rustbot assigned bjorn3 and unassigned wesleywiser Dec 15, 2024

bjorn3 reviewed Dec 15, 2024

View reviewed changes

compiler/rustc_codegen_cranelift/src/compiler_builtins.rs Outdated Show resolved Hide resolved

tgross35 force-pushed the overflowing-c-safe-ret branch from b65b8e5 to b976550 Compare December 17, 2024 10:42